Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: parse userinfo provided in nomad address #423

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: parse userinfo provided in nomad address #423

wants to merge 5 commits into from

Conversation

stind
Copy link

@stind stind commented Sep 13, 2023 •
edited
Loading

Description

I was trying to use nomad-pack with address that includes basic auth, like http://user:pass@host, and received nil pointer dereference error. That's because of the line with conf.HttpAuth.Username = user (conf.HttpAuth may be nil.)

On top of that, I realized that the address is not parsed correctly when it includes both scheme:// and userinfo: scheme://user:pass@addr would be parsed as user = "scheme", pass = "//user:pass", addr = "addr"

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 13, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@stind stind force-pushed the fix-http-auth-from-nomad-addr branch from 9ef14cc to 1da29d1 Compare September 13, 2023 20:19
@stind
fix: parse userinfo provided in nomad address
2430704 
- allow to have a scheme in the address with basic auth
- fix nil point dereference error
@stind stind force-pushed the fix-http-auth-from-nomad-addr branch from 1da29d1 to 2430704 Compare September 13, 2023 20:20
@slonopotamus
Copy link
Contributor

This is kinda... Eh. Maybe just url.Parse?

@stind
Copy link
Author

stind commented Sep 14, 2023

This is kinda... Eh. Maybe just url.Parse?

more than happy to do that 👍 I'll make the change

@angrycub
Copy link
Contributor

angrycub commented Sep 21, 2023 •
edited
Loading

@stind Thanks for the PR. Typically the Nomad API client is configured using the DefaultConfig function of the API package.
Since the Nomad CLI uses NOMAD_HTTP_AUTH to hold the basic authentication information, we do the same for consistency's sake.

However, I think it might be nice to be able to consume all of this from the NOMAD_ADDR itself, so I'd like to talk to some folks here and see if this is something that we can bring into the Nomad API package itself.

Some quick context as to they why they're separate: Nomad itself has never (and still doesn't) support basic authentication itself. The basic authentication "support" was added for folks who are using a auth-enabled proxy in front of Nomad.

@angrycub angrycub self-assigned this Sep 21, 2023
@angrycub angrycub added type/enhancement theme/api Issues related to integration with the Nomad OpenAPI client. labels Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@angrycub angrycub

Labels
theme/api Issues related to integration with the Nomad OpenAPI client. type/enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stind @hashicorp-cla @slonopotamus @angrycub